tests: Cleanup candlepin container by spetrosi · Pull Request #224 · linux-system-roles/rhc

spetrosi · 2025-04-02T15:09:09Z

Enhancement: In always block, import a task to cleanup candlepin container in tests that import the task tests/tasks/setup_candlepin.yml, which starts candlepin container.

Reason: Candlepin container remains after test playbook finishes. If you remove container-selinux rpm, you got selinux denials because the container is running but selinux policies for containers are removed.

spetrosi · 2025-04-02T15:09:17Z

[citest]

spetrosi · 2025-04-02T15:51:05Z

[citest]

spetrosi · 2025-04-02T16:28:04Z

[citest]

spetrosi · 2025-04-02T16:28:50Z

[citest]

spetrosi · 2025-04-02T16:43:02Z

[citest]

spetrosi · 2025-04-02T17:27:31Z

[citest]

spetrosi · 2025-04-02T17:47:34Z

[citest]

spetrosi · 2025-04-03T11:01:28Z

[citest]

spetrosi · 2025-04-03T11:18:20Z

[citest]

spetrosi · 2025-04-03T11:31:27Z

[citest]

spetrosi · 2025-04-03T11:54:45Z

@ptoscano please take a look when you have time

ptoscano · 2025-04-08T11:12:43Z

I'd rename this file as "stop_candlepin.yml"

Since in fact we stop the container to remove it, and since this is specifically for the podman container, I find it clearer to call the file remove_candlepin_container.

ptoscano · 2025-04-08T11:41:55Z

+- name: Check if the candlepin container is started
+  command: podman container exists candlepin
+  register: __rhc_candlepin_cont_started
+  failed_when: false
+  changed_when: false
+
+- name: Stop and potentionally remove Candlepin container
+  command: podman stop candlepin
+  when: __rhc_candlepin_cont_started.rc == 0
+  changed_when: true


Instead of these two tasks, simply use the existing Stop and remove Candlepin container task in setup_candlepin.yml, as it does exactly that.

richm · 2025-04-08T13:45:48Z

@ptoscano re: removing the candlepin image from the local registry

tests should clean up all resources - you are correct that if this is an ephemeral machine, none of the cleanup steps are necessary - what we could do for performance is tag all of those tasks with tests::cleanup, then use --skip-tags tests::cleanup if the machine is ephermeral - but we definitely need the cleanup for the default case, which is non-ephemeral
leaving the container image may cause problems in multi-role tests e.g. the podman tests expect a system without any local containers or images

So ideally, the rhc tests could leave the candlepin image locally until after all of the rhc tests are run, then do any post-rhc tests cleanup, like removing the candlepin image. The problem is that we don't have a way to run a "cleanup-after-all-role-tests-are-completed.yml" playbook.

ptoscano · 2025-04-08T14:12:50Z

what we could do for performance is tag all of those tasks with tests::cleanup, then use --skip-tags tests::cleanup if the machine is ephermeral - but we definitely need the cleanup for the default case, which is non-ephemeral

I'm fine with this. Could we also tweak tox-lsr (which spawns a new system) to also skip that tag by default?

The problem is that we don't have a way to run a "cleanup-after-all-role-tests-are-completed.yml" playbook.

I guess it also depends on how the tests are run, right? ie tox-lsr vs testing-farm vs something else.

richm · 2025-04-08T14:22:42Z

what we could do for performance is tag all of those tasks with tests::cleanup, then use --skip-tags tests::cleanup if the machine is ephermeral - but we definitely need the cleanup for the default case, which is non-ephemeral

I'm fine with this. Could we also tweak tox-lsr (which spawns a new system) to also skip that tag by default?

It's pretty easy to add --skip-tags tests::cleanup to your tox qemu run invocation e.g. tox -e qemu-ansible-core-2.17 -- --image-name centos-10 --log-level debug --skip-tags tests::cleanup -- tests/tests_repositories.yml

There are other ways to use tox e.g. with --make-batch where you do not want to skip cleanup.

The problem is that we don't have a way to run a "cleanup-after-all-role-tests-are-completed.yml" playbook.

I guess it also depends on how the tests are run, right? ie tox-lsr vs testing-farm vs something else.

Right - if we wanted to add something like a "run setup before all role tests" and "run cleanup after all role tests" then we would have to change tox-lsr, testing farm test runner, and the downstream test runners.

spetrosi · 2025-04-16T14:36:39Z

+  when:
+    - '"candlepin" in __rhc_candlepin_container_stopped.stdout'
+
+- name: Remove Candlepin container image


@richm maybe let's not clean up the image? It won't affect anything, and re-downloading it with each test upstream is a waste of network bandwidth.

@richm maybe let's not clean up the image? It won't affect anything, and re-downloading it with each test upstream is a waste of network bandwidth.

Ok. That will affect the podman tests, in the scenarios where we run tests from multiple roles - the podman tests expect no images. I guess that means changing this PR to use __rhc_cleanup_image: false (or just omit __rhc_cleanup_image because the default is false)

I'll fix the podman tests to work if there are images on the system.

spetrosi · 2025-04-16T17:13:36Z

[citest]

richm · 2025-04-16T17:36:35Z

I think just get rid of __rhc_cleanup_image which isn't used anymore, and we can merge

spetrosi · 2025-04-16T17:38:49Z

I'd wait for Pino approval too

spetrosi · 2025-04-16T17:39:46Z

[citest]

ptoscano

Thanks for the updates! I added few more notes that IMHO simplify things a bit, and make the changes more consistent with the current tasks.

Additionally a minor nit common to all the files:

        - name: Clean up Candlepin container
          include_tasks: tasks/remove_candlepin_container.yml

I suggest Clean up Candlepin instead (mimicks the setup tasks), or even Tear down Candlepin (see my previous comment on the naming of the new task file).

Thanks!

ptoscano · 2025-04-23T09:20:18Z

Since this file now only stops candlepin, better name it stop_candlepin.yml, or maybe even better teardown_candlepin.yml (symmetric with "setup")

ptoscano · 2025-04-23T09:22:10Z

Rather than two tasks, simply try to stop the container, and do not consider the missing container an error. In practice simply move here the single task Stop and remove Candlepin container as currently in setup_candlepin.yml.

Now this doesn't work because setup_candlepin.yml has the step to remove the container just in case the container was running from the previous playbook. But role always cleans up the container, and this step would fail. Although, it's good to have the step in setup_candlepin.yml for the case when you are testing locally and might have some container running from the previous test.
I think it's better to keep it the Ansible way and keep the task. By importing the task we say bring the system to the state where this container is removed, whether it exists now or not.

Not sure what you mean here: the existing task that tries to stop the candlepin container also takes care of not erroring out in case the container does not exist. Strictly speaking, doing first the existence check and then stopping it may be a minor TOCTOU issue: https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use.

spetrosi · 2025-04-23T10:34:41Z

[citest]

ptoscano

Thanks for the updates, a couple of final changes, and it should be ready.

Make sure to squash all the commits.

ptoscano · 2025-04-24T09:43:41Z

Not sure what you mean here: the existing task that tries to stop the candlepin container also takes care of not erroring out in case the container does not exist. Strictly speaking, doing first the existence check and then stopping it may be a minor TOCTOU issue: https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use.

In always block, in tests that import the task tests/tasks/setup_candlepin.yml, import a task to cleanup candlepin container

spetrosi · 2025-04-25T11:44:33Z

[citest]

spetrosi requested review from ptoscano and richm as code owners April 2, 2025 15:09

richm reviewed Apr 2, 2025

View reviewed changes

Comment thread tests/tasks/cleanup_candlepin_container.yml Outdated

spetrosi force-pushed the cleanup-candlepin-container branch from 9b93f3a to 918739c Compare April 2, 2025 15:50

richm reviewed Apr 2, 2025

View reviewed changes

Comment thread tests/tasks/cleanup_candlepin_container.yml Outdated

spetrosi force-pushed the cleanup-candlepin-container branch from da8e1bb to 176bf97 Compare April 2, 2025 16:27

spetrosi force-pushed the cleanup-candlepin-container branch from 176bf97 to 3d012f5 Compare April 2, 2025 16:42

richm reviewed Apr 2, 2025

View reviewed changes

Comment thread tests/tasks/cleanup_candlepin_container.yml Outdated

spetrosi force-pushed the cleanup-candlepin-container branch from 3d012f5 to c73c4de Compare April 2, 2025 17:27

spetrosi force-pushed the cleanup-candlepin-container branch from c73c4de to 6bf0844 Compare April 2, 2025 17:47

spetrosi force-pushed the cleanup-candlepin-container branch from 6bf0844 to fe058c2 Compare April 3, 2025 11:01

spetrosi force-pushed the cleanup-candlepin-container branch from fe058c2 to 8b0ef28 Compare April 3, 2025 11:18

spetrosi force-pushed the cleanup-candlepin-container branch from 8b0ef28 to 025dd72 Compare April 3, 2025 11:31

ptoscano requested changes Apr 8, 2025

View reviewed changes

spetrosi commented Apr 16, 2025

View reviewed changes

spetrosi force-pushed the cleanup-candlepin-container branch from 7c4afed to cfcda44 Compare April 16, 2025 17:28

richm reviewed Apr 16, 2025

View reviewed changes

Comment thread tests/tests_environments.yml Outdated

spetrosi force-pushed the cleanup-candlepin-container branch from cfcda44 to 3ad822a Compare April 16, 2025 17:38

spetrosi requested a review from ptoscano April 17, 2025 06:05

ptoscano requested changes Apr 23, 2025

View reviewed changes

spetrosi force-pushed the cleanup-candlepin-container branch 2 times, most recently from 2b558ee to 96c2daa Compare April 23, 2025 10:17

spetrosi force-pushed the cleanup-candlepin-container branch from 96c2daa to 4915a6d Compare April 23, 2025 10:57

spetrosi requested a review from ptoscano April 23, 2025 13:53

richm approved these changes Apr 23, 2025

View reviewed changes

ptoscano requested changes Apr 24, 2025

View reviewed changes

spetrosi force-pushed the cleanup-candlepin-container branch 3 times, most recently from 490b08c to 36a306e Compare April 24, 2025 11:03

tests: Cleanup candlepin container

0f59dc1

In always block, in tests that import the task tests/tasks/setup_candlepin.yml, import a task to cleanup candlepin container

spetrosi force-pushed the cleanup-candlepin-container branch from 36a306e to 0f59dc1 Compare April 24, 2025 11:04

spetrosi requested a review from ptoscano April 24, 2025 11:04

spetrosi merged commit 6ee2eae into linux-system-roles:main Apr 25, 2025
18 checks passed

Conversation

spetrosi commented Apr 2, 2025

Uh oh!

spetrosi commented Apr 2, 2025

Uh oh!

Uh oh!

spetrosi commented Apr 2, 2025

Uh oh!

Uh oh!

spetrosi commented Apr 2, 2025

Uh oh!

spetrosi commented Apr 2, 2025

Uh oh!

spetrosi commented Apr 2, 2025

Uh oh!

Uh oh!

spetrosi commented Apr 2, 2025

Uh oh!

spetrosi commented Apr 2, 2025

Uh oh!

spetrosi commented Apr 3, 2025

Uh oh!

spetrosi commented Apr 3, 2025

Uh oh!

spetrosi commented Apr 3, 2025

Uh oh!

spetrosi commented Apr 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spetrosi Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richm commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ptoscano commented Apr 8, 2025

Uh oh!

richm commented Apr 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spetrosi commented Apr 16, 2025

Uh oh!

Uh oh!

richm commented Apr 16, 2025

Uh oh!

spetrosi commented Apr 16, 2025

Uh oh!

spetrosi commented Apr 16, 2025

Uh oh!

ptoscano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

spetrosi commented Apr 23, 2025

Uh oh!

spetrosi Apr 16, 2025 •

edited

Loading

richm commented Apr 8, 2025 •

edited

Loading